Belos: Adds more unpreconditioned Tpetra tests #9163
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit translates more of the Epetra-based tests to Tpetra linear algebra. 1) The Tpetra solver factory test was extended to include all of the linear solvers that are accessible through the Belos linear solver factory. This resulted in the detection of a bug in the test that was causing solver failures, which has been addressed. 2) Adds diagonal test to MINRES and TFQMR solvers for Epetra. The extension of the Tpetra solver factory test illustrated a problem in the MINRES solver for diagonal/identity matrices, which has been addressed in this commit. 3) Adds BiCGStab, RCG and TFQMR test to Tpetra-based linear algebra. Testing for these solvers has been absent before the extention to the Tpetra solver factory and the addition of these tests.
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
Using Repos:
Pull Request Author: hkthorn |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_python_3
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
2 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
cgcgcg
reviewed
May 24, 2021
| @@ -592,8 +594,8 @@ class MinresIter : virtual public MinresIteration<ScalarType,MV,OP> { | |||
|
|
|||
| // Update x: | |||
| // x = x + phi*w; | |||
| MVT::MvAddMv( one, *cur_soln_vec, phi, *W_, *cur_soln_vec ); | |||
| lp_->updateSolution(); | |||
| //MVT::MvAddMv( one, *cur_soln_vec, phi, *W_, *cur_soln_vec ); | |||
There was a problem hiding this comment.
remove commented line, add comment that updateSolution does this?
There was a problem hiding this comment.
Sure, I can add that when I fix the solver for right-preconditioning.
Comment on lines
-313
to
-317
|
|
||
| # ifdef BELOS_TEUCHOS_TIME_MONITOR | ||
| Teuchos::TimeMonitor::summarize (verbOut); | ||
| # endif // BELOS_TEUCHOS_TIME_MONITOR | ||
|
|
There was a problem hiding this comment.
The timer summary is requested by the solver, so a second timer summary is not necessary.
| try { | ||
| solver->solve (); | ||
| } catch (std::exception& e) { | ||
| out << "*** FAILED: Belos::SolverFactory::solve threw an exception: " |
There was a problem hiding this comment.
Added a try/catch loop around the solve because the test was not catching solver failures. This was useful for debugging the test and solvers that were having issues with identity matrices.
jennloe
approved these changes
May 24, 2021
| @@ -403,12 +403,13 @@ class MinresIter : virtual public MinresIteration<ScalarType,MV,OP> { | |||
| // Create convenience variables for zero, one. | |||
| const ScalarType one = SCT::one(); | |||
| const MagnitudeType zero = SMT::zero(); | |||
| const MagnitudeType m_zero = SMT::zero(); | |||
There was a problem hiding this comment.
How is the m_zero here different from zero?
There was a problem hiding this comment.
It's a zero for the magnitude type. It's easier to identify the difference between the scalar type zero and magnitude type zero by using the "m_" prefix. It is not used consistently throughout the code, but it helps with clarity.
There was a problem hiding this comment.
Did you notice that the 'zero' above here is also a magnitude type?
|
As a side note- It would be nice to add a Tpetra test or two that uses a Matrix Market file. When I play with them, I always get annoyed that our Tpetra tests default to only .hb files, and then I have to put in the Matrix Market reader manually. :) |
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ jennloe ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
cgcgcg
approved these changes
May 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@trilinos/belos
Motivation
This commit translates more of the Epetra-based tests to Tpetra
linear algebra.
The Tpetra solver factory test was extended to include all of
the linear solvers that are accessible through the Belos linear
solver factory. This resulted in the detection of a bug in the
test that was causing solver failures, which has been addressed.
Adds diagonal test to MINRES and TFQMR solvers for Epetra.
The extension of the Tpetra solver factory test illustrated a
problem in the MINRES solver for diagonal/identity matrices,
which has been addressed in this commit.
Adds BiCGStab, RCG and TFQMR test to Tpetra-based linear
algebra. Testing for these solvers has been absent before the
extention to the Tpetra solver factory and the addition of these
tests.
Stakeholder Feedback
Testing
OSX GCC10.x serial